Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write model run outputs #41

Merged
merged 26 commits into from
Oct 8, 2024
Merged

Write model run outputs #41

merged 26 commits into from
Oct 8, 2024

Conversation

zsusswein
Copy link
Collaborator

@zsusswein zsusswein commented Sep 16, 2024

At long last, the output schema PR is ready to go. There are a few actions failing; these should be ignored (see #49). I'm sorry about the size here -- it's really much fewer than ~1200 lines of code, but still not awesome of me.

These are the functions and documentation to go from fitted EpiNow2 model to a directory of outputs ready to be uploaded to Azure Blob. I feel good about all the functionality and that the schema includes most of what we want (in particular both pre- and post-nowcast cases).

It would be particularly helpful to get review on:

  • Clarity and robustness of documentation. Does the readme make clear what is produced and where? I'd like to start pointing people to the readme when they have questions.
  • Output schema. I went with a hybrid format on task-grouped and output-grouped. I think it'll make it easier to both glob over files and do manual investigation, but open to feedback here. Does anyone foresee issues with extensibility if there's a new output type we want? Or total dealbreakers if/when plugging in a new type of model?
  • Missing outputs. I tried to be reasonably comprehensive for everything we currently use. It'd be good to get feedback if there's something important missing.

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files

📢 Thoughts on this report? Let us know!

@zsusswein zsusswein force-pushed the zs-write-output branch 3 times, most recently from b329b8f to 4681589 Compare September 16, 2024 20:42
R/write_output.R Outdated Show resolved Hide resolved
@zsusswein zsusswein force-pushed the zs-write-output branch 3 times, most recently from 02fec28 to cb634e4 Compare October 1, 2024 20:39
DESCRIPTION Show resolved Hide resolved
@zsusswein zsusswein marked this pull request as ready for review October 1, 2024 20:54
Copy link
Collaborator

@natemcintosh natemcintosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great!

Had one or two questions, but overall love the high code quality, general readability, and the nice thick set of tests.

R/write_output.R Show resolved Hide resolved
R/write_output.R Show resolved Hide resolved
R/write_output.R Show resolved Hide resolved
README.md Show resolved Hide resolved
tests/testthat/test-write_output.R Show resolved Hide resolved
tests/testthat/test-write_output.R Outdated Show resolved Hide resolved
@athowes
Copy link
Collaborator

athowes commented Oct 2, 2024

Note on this that I don't know that there is an issue describing the plan for this PR (?) so in reviewing I am somewhat unsure what the goals are.

Copy link
Collaborator

@athowes athowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part way through.

R/extract_diagnostics.R Outdated Show resolved Hide resolved
R/extract_diagnostics.R Outdated Show resolved Hide resolved
R/extract_diagnostics.R Outdated Show resolved Hide resolved
R/extract_diagnostics.R Outdated Show resolved Hide resolved
R/extract_diagnostics.R Show resolved Hide resolved
R/extract_diagnostics.R Show resolved Hide resolved
R/extract_diagnostics.R Outdated Show resolved Hide resolved
R/extract_diagnostics.R Outdated Show resolved Hide resolved
R/extract_diagnostics.R Show resolved Hide resolved
R/extract_diagnostics.R Show resolved Hide resolved
Copy link
Collaborator

@kgostic kgostic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm halfway through. Will resume later

R/extract_diagnostics.R Outdated Show resolved Hide resolved
R/extract_diagnostics.R Show resolved Hide resolved
R/extract_diagnostics.R Outdated Show resolved Hide resolved
R/extract_diagnostics.R Show resolved Hide resolved
R/extract_diagnostics.R Outdated Show resolved Hide resolved
R/write_output.R Show resolved Hide resolved
R/write_output.R Show resolved Hide resolved
R/write_output.R Outdated Show resolved Hide resolved
R/write_output.R Show resolved Hide resolved
R/write_output.R Show resolved Hide resolved
@zsusswein zsusswein requested a review from athowes October 3, 2024 12:59
Copy link
Collaborator

@athowes athowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like good things happening here. Somewhat hard to say with how large it is

R/write_output.R Show resolved Hide resolved
R/write_output.R Show resolved Hide resolved
R/write_output.R Outdated Show resolved Hide resolved
R/write_output.R Show resolved Hide resolved
R/write_output.R Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
R/write_output.R Show resolved Hide resolved
R/write_output.R Show resolved Hide resolved
Copy link
Collaborator

@kgostic kgostic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to finish going through test-write-output.R, but I don't have any blocking concerns at this point!

R/extract_diagnostics.R Show resolved Hide resolved
R/extract_diagnostics.R Show resolved Hide resolved
R/extract_diagnostics.R Show resolved Hide resolved
R/extract_diagnostics.R Show resolved Hide resolved
R/extract_diagnostics.R Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
tests/testthat/helper-write_parameter_file.R Show resolved Hide resolved
tests/testthat/test-extract_diagnostics.R Show resolved Hide resolved
@zsusswein
Copy link
Collaborator Author

The failing check is a known, unrelated issue. It's being handled in #60

@zsusswein zsusswein merged commit edb4fbd into main Oct 8, 2024
9 checks passed
@zsusswein zsusswein deleted the zs-write-output branch October 8, 2024 13:07
zsusswein added a commit that referenced this pull request Oct 15, 2024
* Ignore CI/CD stuff in Rbuildignore

* Extract diagnostics from fitted model

* Basic output schema

* Use `.pre-commit.config.yaml` from main

To fix weirdness with unicode parsing error from.....somewhere?

* Update output schema

* Bump NEWS

* Bump NEWS

* Expand on readme

* Use setequal for column name checks

h/t @natemcintosh

* Apply suggestions from code review

Co-authored-by: Adam Howes <[email protected]>

* Update with Adam's review

* Update R/write_output.R

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update R/extract_diagnostics.R

Co-authored-by: Katie Gostic (she/her) <[email protected]>

* Add alert with dates for low case count diagnostic

* Apply suggestions from code review

Co-authored-by: Adam Howes <[email protected]>

* Use new R-universe Stan repository

* Update README.md

Co-authored-by: Katie Gostic (she/her) <[email protected]>

* Update README.md

Co-authored-by: Katie Gostic (she/her) <[email protected]>

* Condense dir creation

* Expose quantiles for summarization

* Save the description of the different EpiNow2 params

* Add comment explaining why dates work

* Clarify comment on EpiNow2 param outputs

* Add `reports` to output

---------

Co-authored-by: Adam Howes <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Katie Gostic (she/her) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants